Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New recipe: MAVSDK #25949

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

New recipe: MAVSDK #25949

wants to merge 1 commit into from

Conversation

adings19
Copy link

MAVSDK is a collection of libraries for various programming languages to interface with MAVLink systems such as drones, cameras or ground systems.

Summary

Fixes #22707

Details

This is a new package intended for those who wish to be able to pull MAVSDK from conan center.

I am new to conan (and new to contributing to open source projects!) so all help and guidance is welcome.


MAVSDK is a collection of libraries for various programming languages
to interface with MAVLink systems such as drones, cameras or ground
systems.
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks a lot for taking the time to create this recipe and the PR, we really appreciate it :)

I've taken an initial look and most things look good, only a few structural changes are necessary, happy to help if needed

Comment on lines +55 to +60
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True)
sources = self.conan_data["sources"][self.version]["mavlink"]
mavlink_dir = join(self.source_folder, "src", "third_party",
"mavlink", "include", "mavlink", "v2.0")
git = Git(self, folder=mavlink_dir)
git.fetch_commit(sources["url"], sources["commit"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True)
sources = self.conan_data["sources"][self.version]["mavlink"]
mavlink_dir = join(self.source_folder, "src", "third_party",
"mavlink", "include", "mavlink", "v2.0")
git = Git(self, folder=mavlink_dir)
git.fetch_commit(sources["url"], sources["commit"])
get(self, **self.conan_data["sources"][self.version]["sdk"], strip_root=True)
sources = self.conan_data["sources"][self.version]["mavlink"]
mavlink_dir = join(self.source_folder, "src", "third_party",
"mavlink", "include", "mavlink", "v2.0")
git = Git(self, folder=mavlink_dir)
git.fetch_commit(sources["url"], sources["commit"])

I'd like to understand a bit better the relationship between the sdk and the c library. When building the SDK, why do we need to provide the library separatedly? Is it a dependency? If so, maybe the best approach would be to split this into two - the c library, and the SDK that depends on it

Also, using git repositories in CCI is not a good idea, we should move the library to download a file just like the SDK does. If the library does not provide tags, we can still create versions from the commits themselves if needed :)

Comment on lines +76 to +77
replace_in_file(self, join(self.source_folder, f),
"JsonCpp::jsoncpp", "JsonCpp::JsonCpp")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be better fixed by setting this in the generate method:

deps = CMakeDeps(self)
deps.set_property("jsoncppp", "cmake_target_name", "JsonCpp::jsoncpp")
deps.generate()

This way Conan will change the generated target name to match those used by the project, removing the need to patch the sources themselves!


# TODO: create instead patch files to simplify building versions
replace_in_file(self, join(self.source_folder, "CMakeLists.txt"),
'message(STATUS "Version: ${VERSION_STR}")',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? How does this help the build process?

Either way, this might be better suited to be added as part of the CMakeToolchain variables if the variable is needed, something like this in the generate method if this is 100% needed:

tc = CMakeToolchain(self)
tc.variables["VERSION_STR"] = f"v{self.version}"

Comment on lines +43 to +45
self.requires("jsoncpp/[>=1.9.5 <2]")
self.requires("tinyxml2/[>=9.0.0 <10]")
self.requires("libcurl/[>=7.86.0 <8]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now version ranges are limited to a select set of libraries, check https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges, which means that of these 3, only libcurl can use version ranges. Sorry for the confussion :)

Also, if curl's 7.86 is the minimunm required, then great! But the lower the range, the more likely this won't cause issues to other elements on the graph (We have [>=7.78 <9] documented as the desired range by default, for example)

Comment on lines +35 to +37
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def config_options(self):
if self.settings.os == "Windows":
del self.options.fPIC
implements = ["auto_shared_fpic"]

will take care of this, and the missing configure() method that handles fpic values when building shared libraries

class MAVSDKConan(ConanFile):
name = "mavsdk"
license = "BSD-3-Clause"
author = "SINTEF Ocean"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
author = "SINTEF Ocean"

authors in CCI recipes don't have a clear meaning, so we don't add them

Comment on lines +102 to +103
if self.options.shared:
copy(self, "*mavsdk*.dll", dst=join(self.package_folder, "bin"), src=join(self.build_folder, "src"), keep_path=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the cmake installation not performing this step? How come?

Comment on lines +1 to +6
#include <mavsdk.h>

int main()
{
mavsdk::Mavsdk mavsdk;
std::string version = mavsdk.version();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <mavsdk.h>
int main()
{
mavsdk::Mavsdk mavsdk;
std::string version = mavsdk.version();
#include <mavsdk.h>
#include <iostream>
int main()
{
mavsdk::Mavsdk mavsdk;
std::string version = mavsdk.version();
std::cout << "MAVSDK version: " << version << std::endl;

To ensure the compiler does not optimize these calls away

'message(STATUS "Version: ${VERSION_STR}")',
f'set(VERSION_STR v{self.version})\nmessage(STATUS "Version: ${{VERSION_STR}}")')

replace_in_file(self, join(self.source_folder, "src/core/connection.h"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a fix for an issue known upstream? Has it been reported? Having a link for traceability would be great!

@AbrilRBS AbrilRBS self-assigned this Nov 14, 2024
@@ -0,0 +1,8 @@
sources:
"0.39.0":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this version specifically? It's over 3 years old with version 2 already out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[request] MAVSDK/2.1.0
2 participants